Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

glibc: use libgcc from pkgsHostHost #259964

Closed
wants to merge 1 commit into from

Conversation

alyssais
Copy link
Member

@alyssais alyssais commented Oct 9, 2023

Description of changes

glibc depends on buildPackages.glibc for locale things.

buildPackages.glibc depended on buildPackages.libgcc, which, since it's GCC, depends on the target's bintools, which depend on the target's glibc, which, again, depends on buildPackages.glibc, causing an infinute recursion when evaluating buildPackages.glibc when glibc hasn't come from stdenv (e.g. on musl).

It's unlikely that buildPackages.glibc needs anything to do with the target, so I've broken the cycle by having it use
pkgsHostHost.libgcc.

This fixes cross compilation from musl to glibc.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

glibc depends on buildPackages.glibc for locale things.

buildPackages.glibc depended on buildPackages.libgcc, which, since
it's GCC, depends on the target's bintools, which depend on the
target's glibc, which, again, depends on buildPackages.glibc, causing
an infinute recursion when evaluating buildPackages.glibc when glibc
hasn't come from stdenv (e.g. on musl).

It's unlikely that buildPackages.glibc needs anything to do with the
target, so I've broken the cycle by having it use
pkgsHostHost.libgcc.

This fixes cross compilation from musl to glibc.
@alyssais alyssais added 6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: musl Running or building packages with musl libc labels Oct 9, 2023
@alyssais alyssais requested review from Ericson2314, a user and yu-re-ka October 9, 2023 10:19
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 9, 2023
@Ericson2314
Copy link
Member

This whole situation makes me sad... #132343 really needs to be finished.

@ghost
Copy link

ghost commented Oct 22, 2023

This whole situation makes me sad... #132343 really needs to be finished.

Hey at least I got mainline nixpkgs to use at least one separately-built target library. Previously the record was zero. One step at a time...

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

But do we really need to rename the parameter here? The diff is much smaller without the rename from libgcc to libgccHostHost.

It's unlikely that buildPackages.glibc needs anything to do with the target

Indeed. Although a more direct fix might be to simply define the top-level libgcc to be if host!=target then pkgsHostHost.libgcc else callPackage ... to fix the problem at the source.

I have general feeling that there is a whole class of packages (the overwhelming majority) where this sort of thing ought to be done, because the package has no meaningful notion of "target platform". In fact in the long run I think that ought to be the default, and packages ought to have to declare that they have a meaningful target platform in order to avoid this behavior. But I digress...

This fixes cross compilation from musl to glibc.

Could you let me know the attrname you use as a reproducer test for this bug? It would make a nice addition to tests.cross.sanity, which is my collection of cross-compilation smoke tests (since we still don't have meaningful CI for the vast majority of cross compilation configurations). You seem to come across a lot of these interesting corner cases; don't hesitate to add them there if you think there isn't any existing coverage.

pkgs/development/libraries/glibc/default.nix Show resolved Hide resolved
pkgs/development/libraries/glibc/default.nix Show resolved Hide resolved
pkgs/development/libraries/glibc/default.nix Show resolved Hide resolved
pkgs/development/libraries/glibc/default.nix Show resolved Hide resolved
pkgs/top-level/all-packages.nix Show resolved Hide resolved
@alyssais
Copy link
Member Author

But do we really need to rename the parameter here? The diff is much smaller without the rename from libgcc to libgccHostHost.

I had it that way originally, but I decided it was confusing to keep it with the same name, because it could end up being passed in through callPackage automatically, which wouldn't be correct. I think it's better to be explicit and avoid callPackage naming conflicts.

@alyssais
Copy link
Member Author

alyssais commented Oct 22, 2023

Could you let me know the attrname you use as a reproducer test for this bug?

pkgsMusl.pkgsCross.gnu64.hello on x86_64-linux is a reproducer

@ghost
Copy link

ghost commented Oct 23, 2023

@alyssais thank you so much for tracking down this circular dependency! However breaking the chain by taking libgcc from pkgsHostHost adds considerable additional complexity (which really should be explained in the codebase if we go that route -- as it stands this PR doesn't add anything to the code explaining why libgccHostHost is used, nor why pkgsHostHost.libgcc doesn't fix the recursion).

Would you consider this alternative fix? (Note I am still building it to verify that it works):

IMHO the alternative adds no extra complexity, and actually makes libgcc/default.nix easier to understand. I have however added the circularity you discovered as a comment there to warn anybody who thinks of changing it back. I'm a bit suspicious of the original reasoning for using pkgsBuildHost instead of pkgsBuildBuild; glibc is a pretty fast compile and unless you're starting from bootstrap-files/musl.nix it's already built anyways.

ghost pushed a commit that referenced this pull request Oct 26, 2023
This is an alternative resolution of the problem identified in

  #259964

which stated that "glibc depends on buildPackages.glibc for locale
things.  buildPackages.glibc depended on buildPackages.libgcc,
which, since it's GCC, depends on the target's bintools, which
depend on the target's glibc, which, again, depends on
buildPackages.glibc, causing an infinute recursion when evaluating
buildPackages.glibc when glibc hasn't come from stdenv (e.g. on
musl)."

The fact that we use pkgsBuildHost.glibc instead of
pkgsBuildBuild.glibc to generate the locales has always been a gross
hack.  If we simply remove the gross hack the circularity goes away.
@Ericson2314
Copy link
Member

Closing this because that one is merged.

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 26, 2023

In fact in the long run I think that ought to be the default, and packages ought to have to declare that they have a meaningful target platform in order to avoid this behavior. But I digress...

Yes we really need to do this. Let's make an issue for it. #263640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: musl Running or building packages with musl libc 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants